Require STDIN to be specified explicitly with `-` #14

jasonkarns · 2016-06-18T12:35:01Z

First step for #5

ztombol

Thanks for the additional cleanup work too!

ztombol · 2016-09-25T19:27:19Z

test/50-assert-16-refute_output.bats

-  run echo 'a'
-  run refute_output <<INPUT
+@test 'refute_output() - : reads <unexpected> from STDIN' {
+  run echo '-'


Why did you change the run command? As far as I can tell, it worked with run echo 'a' as well.

As a stronger assertion that - as input on stdin doesn't cause problems. (Since the - is also the flag to the assertion)

Ah, okay got it. Thanks!

ztombol

Few minor changes and ready to merge.

ztombol · 2016-10-29T15:39:06Z

src/assert.bash

@@ -185,12 +185,14 @@ assert_failure() {
 assert_output() {


Could you update the function comment too? For example:

# Options: # -p, --partial - partial matching # -e, --regexp - extended regular expression matching +# - - read expected output from the standard input # Arguments: -# $1 - [=STDIN] expected output +# $1 - expected output # Returns: # 0 - expected matches the actual output # 1 - otherwise

ztombol · 2016-10-29T15:47:39Z

src/assert.bash

@@ -278,12 +282,14 @@ assert_output() {
 refute_output() {


Could you update the function comment too? For example:

# Options: # -p, --partial - partial matching # -e, --regexp - extended regular expression matching +# - - read unexpected output from the standard input # Arguments: -# $1 - [=STDIN] unexpected output +# $1 - unexpected output # Returns: # 0 - unexpected matches the actual output # 1 - otherwise

ztombol · 2016-10-29T15:49:42Z

src/assert.bash


  # Handle options.
  while (( $# > 0 )); do
    case "$1" in
      -p|--partial) is_mode_partial=1; shift ;;
      -e|--regexp) is_mode_regexp=1; shift ;;
+      -) use_stdin=1; shift ;;


I'm thinking a long option, e.g. --stdin, would be nice. Safe for refute_output. What do you think?

jasonkarns · 2016-11-08T21:52:45Z

Updated comments and added --stdin longform option to both assert_output and refute_output

ztombol

Needs a few more simple changes and ready to go!

ztombol · 2016-11-21T16:18:20Z

README.md

@@ -189,12 +189,13 @@ By default, literal matching is performed. The assertion fails if
 }
 ```

-The expected output can be specified with a heredoc or standard input as well.
+The expected output can be specified with a heredoc or standard input as well,
+by providing `-` as an option.


Could you mention the long option in the README as well. Like we did for other options:

Partial matching can be enabled with the --partial option (-p for short). When used, the assertion fails if the expected substring is not found in $output.

added! 581aa70

ztombol · 2016-11-21T16:19:37Z

README.md

@@ -287,12 +288,13 @@ By default, literal matching is performed. The assertion fails if
 }
 ```

-The unexpected output can be specified with a heredoc or standard input as well.
+The unexpected output can be specified with a heredoc or standard input as well,
+by providing `-` as an option.


Could you mention the long option in the README as well. Like we did for other options:

Partial matching can be enabled with the --partial option (-p for short). When used, the assertion fails if the expected substring is not found in $output.

added! 581aa70

ztombol · 2016-11-21T16:43:13Z

test/50-assert-15-assert_output.bats

@@ -26,9 +26,19 @@ load test_helper
  [ "${lines[3]}" == '--' ]
 }

-@test 'assert_output(): reads <expected> from STDIN' {
+@test 'assert_output() - : reads <expected> from STDIN' {


Please factor out common parts of tests to avoid duplicating code. Like it's done here.

I actually have a strong dislike of that extraction. Generally I favor non-dry tests because tests should be explicit and self-contained. In this case (and with test_r_regexp), it's quite difficult to understand what's going on with the extracted function. Instead of the test functioning as example usage, we now have variables (unnamed vars at that) being used in place of the very thing that makes the test unique.

If anything should be extracted, I'd extract the duplicated assertions from all tests:

[ "$status" -eq 0 ] [ "${#lines[@]}" -eq 0 ]

The implementation of those statements are not germane to the test, so abstracting them away can improve readability. However, extracting away the actual invocation is a bad idea, IMO. The unique invocation is literally the raison d'être for the test(s).

Opened a PR to this PR to show usage of just the assertion helper rather than attempting to dry up the entire test(s).

bats-core#1

ztombol · 2016-11-21T16:44:00Z

test/50-assert-15-assert_output.bats

+  run assert_output - <<STDIN
+a
+STDIN
+echo "$output"


Leftover debug print.

ztombol · 2016-11-21T16:47:17Z

test/50-assert-15-assert_output.bats

+
+@test 'assert_output() --stdin : reads <expected> from STDIN' {
+  run echo 'a'
+  run assert_output --stdin <<STDIN
 a
 STDIN
 echo "$output"


Can you remove this debug print left over from a previous PR?

done adc1c7b

jasonkarns · 2018-11-22T13:29:10Z

Merged this branch in my fork: bats-core@658b73a

jasonkarns added 3 commits June 18, 2016 08:33

stdin must be specified explicitly with -

1a75841

drop early return

b4e90ad

docs

0fb9d2a

jasonkarns mentioned this pull request Jun 18, 2016

Assert/Refute against nonempty/empty output #15

Open

ztombol reviewed Sep 25, 2016

View reviewed changes

ztombol requested changes Oct 29, 2016

View reviewed changes

ztombol mentioned this pull request Nov 8, 2016

Publish to npm #8

Closed

jasonkarns added 2 commits November 8, 2016 16:41

Update code comments

ae447cd

Allow longform --stdin option as well

0dc0b49

ztombol requested changes Nov 21, 2016

View reviewed changes

jasonkarns added 2 commits November 22, 2016 16:23

Document stdin option in readme

581aa70

Remove leftover debugging lines

adc1c7b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Require STDIN to be specified explicitly with `-` #14

Require STDIN to be specified explicitly with `-` #14

jasonkarns commented Jun 18, 2016 •

edited

Loading

ztombol left a comment

ztombol Sep 25, 2016 •

edited

Loading

jasonkarns Sep 26, 2016

ztombol Oct 29, 2016

ztombol left a comment

ztombol Oct 29, 2016

ztombol Oct 29, 2016

ztombol Oct 29, 2016

jasonkarns commented Nov 8, 2016

ztombol left a comment

ztombol Nov 21, 2016

jasonkarns Nov 22, 2016

ztombol Nov 21, 2016

jasonkarns Nov 22, 2016

ztombol Nov 21, 2016

jasonkarns Nov 22, 2016

jasonkarns Nov 22, 2016

ztombol Nov 21, 2016

ztombol Nov 21, 2016

jasonkarns Nov 22, 2016

jasonkarns commented Nov 22, 2018

Require STDIN to be specified explicitly with - #14

Are you sure you want to change the base?

Require STDIN to be specified explicitly with - #14

Conversation

jasonkarns commented Jun 18, 2016 • edited Loading

ztombol left a comment

Choose a reason for hiding this comment

ztombol Sep 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ztombol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonkarns commented Nov 8, 2016

ztombol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonkarns commented Nov 22, 2018

Require STDIN to be specified explicitly with `-` #14

Require STDIN to be specified explicitly with `-` #14

jasonkarns commented Jun 18, 2016 •

edited

Loading

ztombol Sep 25, 2016 •

edited

Loading